Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for device dehydration v2 #12316

Merged
merged 22 commits into from
Apr 15, 2024
Merged

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Mar 5, 2024

Rehydrates/dehydrates device if dehydration is enabled in .well-known file (or if there is a dehydration key in SSSS). (Currently, js-sdk only supports dehydration when using Rust crypto.)

Dehydrated devices are hidden or displayed specially when they are cross-signed, and if there is only one. Otherwise, they are displayed as normal devices.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@uhoreg uhoreg added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Mar 26, 2024
@t3chguy
Copy link
Member

t3chguy commented Apr 10, 2024

Looks like SonarCloud doesn't consider the playwright tests?

Correct, because even just launching the app would hit like 30% overall test coverage without any useful assertions

Comment on lines 24 to 25
* Dehydration can only be enabled if encryption is available, and the crypto
* backend supports dehydration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Dehydration can only be enabled if encryption is available, and the crypto
* backend supports dehydration.
* Note that this doesn't necessarily mean that device dehydration has been initialised
* (yet) on this client; rather, it means that the server supports it, the crypto backend
* supports it, and the application configuration suggests that it *should* be
* initialised on this device.

src/utils/device/dehydration.ts Outdated Show resolved Hide resolved
src/utils/device/dehydration.ts Outdated Show resolved Hide resolved
src/utils/device/dehydration.ts Outdated Show resolved Hide resolved
Comment on lines 158 to 159

await initializeDehydration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I suggest you make an issue about it if you don't expect to have it done in very short order.

@richvdh
Copy link
Member

richvdh commented Apr 10, 2024

As @t3chguy implies: I'm afraid you'll need to write jest tests to get the coverage up. Hopefully it should be pretty easy.

@uhoreg
Copy link
Member Author

uhoreg commented Apr 11, 2024

Yup, I'll try to write some stuff to make SonarCloud happy

src/utils/device/dehydration.ts Outdated Show resolved Hide resolved
Comment on lines 158 to 159

await initializeDehydration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@uhoreg Did you get a chance to make an issue?

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The tests look very comprehensive!

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@uhoreg uhoreg enabled auto-merge April 15, 2024 15:29
@uhoreg uhoreg added this pull request to the merge queue Apr 15, 2024
Merged via the queue into matrix-org:develop with commit 3137339 Apr 15, 2024
19 checks passed
@uhoreg uhoreg deleted the dehydration_v2 branch April 15, 2024 16:08
@t3chguy
Copy link
Member

t3chguy commented Jun 12, 2024

This seems to have bypassed labs but relies on experimental MSC implementations, this is quite unexpected

@uhoreg
Copy link
Member Author

uhoreg commented Jun 12, 2024

This seems to have bypassed labs but relies on experimental MSC implementations, this is quite unexpected

It has to be enabled on the server by editing the homeserver.yaml, and needs something added to the server's .well-known file, so I didn't think it needed any more configuration to turn it on. Also, a bunch of it runs on login, before the user is able to enable the labs flag.

@t3chguy
Copy link
Member

t3chguy commented Jun 12, 2024

before the user is able to enable the labs flag.

Sure, that's why we support setting labs flags in config.json - normally the product policy is to not release things relying on unstable APIs without product sign off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants